Skip to content

Conversation

@cthumuluru-crdb
Copy link
Contributor

@cthumuluru-crdb cthumuluru-crdb commented Nov 18, 2025

DRPC is currently disabled by default and can be enabled via the rpc.experimental_drpc.enabled cluster setting. Some code paths, such as command-line tools, don't have access to this setting. This change introduces a --use-new-drpc flag for start commands to enable DRPC in those cases. This change enables the use of DRPC for the node join RPC when the --use-new-rpc flag is set, and defaults to gRPC otherwise.

Epic: CRDB-55200, CRDB-51459
Fixes: #155592, #149600
Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Nov 18, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cthumuluru-crdb cthumuluru-crdb added the do-not-merge bors won't merge a PR with this label. label Nov 18, 2025
@cthumuluru-crdb cthumuluru-crdb changed the title wip: Issue 149600 rpc, server: add DRPC support for node join RPC Nov 18, 2025
@cthumuluru-crdb cthumuluru-crdb changed the title rpc, server: add DRPC support for node join RPC rpc, server: add DRPC support for Node join RPC Nov 18, 2025
@cthumuluru-crdb cthumuluru-crdb changed the title rpc, server: add DRPC support for Node join RPC rpc, server: add DRPC support for node join RPC Nov 18, 2025
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@github-actions github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 18, 2025
@cthumuluru-crdb cthumuluru-crdb added O-AI-Review-Not-Helpful AI reviewer produced result which was incorrect or unhelpful and removed o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. labels Nov 18, 2025
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@github-actions github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 20, 2025
@cockroach-teamcity cockroach-teamcity added the X-perf-gain Microbenchmarks CI: Added if a performance gain is detected label Nov 20, 2025
@cthumuluru-crdb cthumuluru-crdb force-pushed the issue-149600 branch 2 times, most recently from 753afff to bd11b0a Compare November 20, 2025 18:38
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

DRPC is currently disabled by default and can be enabled via the
`rpc.experimental_drpc.enabled` cluster setting. Some code paths, such as
command-line tools, don't have access to this setting. This change introduces
a `--use-new-drpc` flag for start commands to enable DRPC in those cases.

Epic: CRDB-55200
Fixes: cockroachdb#155592
Release note: None
This change enables the use of DRPC for the node join RPC when the
`--use-new-rpc` flag is set, and defaults to gRPC otherwise.

Epic: CRDB-51459
Fixed: cockroachdb#149600
Release note: None
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@cthumuluru-crdb cthumuluru-crdb marked this pull request as ready for review November 24, 2025 06:52
@cthumuluru-crdb cthumuluru-crdb requested review from a team as code owners November 24, 2025 06:52
@cthumuluru-crdb cthumuluru-crdb requested review from a team as code owners November 24, 2025 06:52
@github-actions
Copy link

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

Copy link
Contributor

@shubhamdhama shubhamdhama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I wish some non-essential refactor of drpc.go could be done in a separate commit even if that's done in this PR. But now I have gone through the PR so it's fine here.

Comment on lines +391 to +396
// TODO(server): unexport this once dial methods are added in rpccontext.
func DialDRPC(
rpcCtx *Context,
) func(ctx context.Context, target string, _ rpcbase.ConnectionClass, additionalOpts ...drpcclient.DialOption) (drpc.Conn, error) {
return rpcCtx.drpcDialRaw
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed now?

)

var envExperimentalDRPCEnabled = envutil.EnvOrDefaultBool("COCKROACH_EXPERIMENTAL_DRPC_ENABLED", false)
var envExperimentalDRPCEnabled = envutil.EnvOrDefaultBool("COCKROACH_EXPERIMENTAL_DRPC_ENABLED", true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the PR has do-not-merge label, still maintaining a checklist as a separate comment for things we should revert before we remove that label would be good.

Comment on lines 49 to 56
func (c *closeEntirePoolConn) Close() error {
_ = c.Conn.Close()
return c.pool.Close()
if c.closed.CompareAndSwap(false, true) {
_ = c.Conn.Close()
return c.pool.Close()
}

return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in a separate commit.

}()
initClient = kvpb.NewDRPCNodeClient(conn)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract this to a new method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge bors won't merge a PR with this label. O-AI-Review-Not-Helpful AI reviewer produced result which was incorrect or unhelpful o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. X-perf-gain Microbenchmarks CI: Added if a performance gain is detected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

drpc: add a CLI flag to cockroach start command to enable DRPC

3 participants